fix(tls): enforce signature provider contracts - #11
Conversation
🎉 All green!🛠️ No new code quality issues 🔗 Commit SHA: ecc0b81 | Docs | Datadog PR Page | Give us feedback! |
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
thieman
changed the title
thieman
changed the title
| #[test] | ||
| fn tls12_ecdsa_sign_schemes_do_not_advertise_ed25519() { | ||
| assert!(!ECDSA_SCHEMES.contains(&SignatureScheme::ED25519)); | ||
| } |
There was a problem hiding this comment.
This test is pointless, remove it
| const GCM_IMPLICIT_NONCE_LENGTH: usize = 4; | ||
|
|
||
| static ECDSA_SCHEMES: &[SignatureScheme] = &[ | ||
| SignatureScheme::ED25519, |
There was a problem hiding this comment.
It's not actually supported, and GPT claims that's fine from a FIPS perspective. This removal stops advertising it as supported.
There was a problem hiding this comment.
Right, yeah, like this actually seems bad... like fundamentally not possible since the whole thing is that ED25519 is a different scheme from ECDSA.
There was a problem hiding this comment.
[GPT 5.5] Evaluated against the current provider: Ed25519 is widely used, but this CNG-backed provider does not implement Ed25519 signing or verification (src/signer/ec.rs explicitly notes CNG does not support it here, and src/verify.rs has Ed25519 commented out). So I kept the removal from the TLS 1.2 ECDSA scheme list; advertising it would imply a capability the provider cannot satisfy.
| const RSA_MIN_MODULUS_BITS: usize = 2048; | ||
| const RSA_MAX_MODULUS_BITS: usize = 8192; |
There was a problem hiding this comment.
Can we have the agent try and provide first-party documentation sources for stuff like this?
There was a problem hiding this comment.
[GPT 5.5] Added a source note in cddb6ca. The first-party source I used is rustls-webpki itself: its RSA verification algorithms are named/defined for 2048-8192-bit keys, e.g. RSA_PKCS1_2048_8192_SHA256 and the matching PSS variants in rustls-webpki-0.103.13/src/ring_algs.rs / aws_lc_rs_algs.rs. This provider now mirrors that policy before passing RSA public keys to CNG.
There was a problem hiding this comment.
[GPT 5.5] Updated in d132442 to include a direct source link in the code comment. It now points at rustls-webpki 0.103.13’s first-party RSA verification algorithm definitions for 2048-8192-bit keys: https://docs.rs/rustls-webpki/0.103.13/src/rustls_webpki/aws_lc_rs_algs.rs.html#162-182
There was a problem hiding this comment.
It took three tries, but it got there.
| RSA_PSS_SHA512, | ||
| RSA_PSS_SHA384, | ||
| RSA_PSS_SHA256, | ||
| // RFC 4055 requires accepting sha*WithRSAEncryption AlgorithmIdentifiers both with |
There was a problem hiding this comment.
Similar-ish content about linking to first-party sources... maybe the specific section in the RFC?
There was a problem hiding this comment.
[GPT 5.5] Added a more specific source note in cddb6ca. RFC 4055 section 2.1 says implementations MUST accept both NULL and absent parameters as legal/equivalent encodings for these hash AlgorithmIdentifiers. rustls-webpki also registers both present-parameter and _ABSENT_PARAMS RSA PKCS#1 variants, so this mirrors the built-in provider behavior.
There was a problem hiding this comment.
[GPT 5.5] Updated in d132442 to include direct links in the code comment: RFC 4055 section 2.1 for accepting NULL and absent parameters (https://www.rfc-editor.org/rfc/rfc4055.html#section-2.1), plus rustls-webpki’s _ABSENT_PARAMS algorithms (https://docs.rs/rustls-webpki/0.103.13/src/rustls_webpki/aws_lc_rs_algs.rs.html#186-245).
2 participants
Summary
Fixes #8, narrowed to the minimal provider-contract changes:
The broader hardening items from #8 (AEAD in-place aliasing strategy, private-key import zeroization, TLS 1.2 oversized plaintext handling, and runtime panic cleanup) are intentionally deferred for smaller follow-up PRs with focused review.
Testing
Local macOS validation:
cargo fmt -- --checkcargo check --target x86_64-pc-windows-msvccargo clippy --target x86_64-pc-windows-msvc -- -D warningsgit diff --checkWindows runtime validation is expected from GitHub Actions.
Notes for reviewers
This PR now changes only
src/verify.rsandsrc/tls12.rsrelative tomain.